-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run the test suite as a GitHub Action #208
base: main
Are you sure you want to change the base?
Conversation
3fbdfa8
to
207e9dc
Compare
Required on headless systems.
scad_file_path: Path | ||
openscad_binary_path: Path | ||
image_folder_base: Path | ||
parameters: Optional[dict] | ||
'''If set, a temporary parameter file is created, and used with these variables''' | ||
|
||
WINDOWS_DEFAULT_PATH = 'C:\\Program Files\\OpenSCAD\\openscad.exe' | ||
LINUX_DEFAULT_PATH = Path('/bin/openscad') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be just openscad, no need to add the /bin/
prefix as Linux resolves the path by whatever you put in env.PATH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it in cases where you're not running through a shell?
Edit: I also don't think the automatic expansion works when doing the exists()
check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a fan of the absolute path as it won't depend on the users environment. If something is by default on Linux it does not mean we need to depend on it when it is not necessary. Defaults van change, users can do weird shit. We already have a big dependency on openscad. If they change the install location, it fails everywhere and we can change it.
- name: Install OpenSCAD | ||
run: | | ||
sudo apt-get update | ||
sudo apt-get install openscad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This most likely installs the v2021 version, which is quite old. It might be beneficial to either install/use a nightly (appimage) version or even better use openscad's docker container for these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree it is very old but it is the latest stable afaik.
Maybe it is possible to use a fixed "nightly" version? I hate the ci/cd breaking due to the nightly being updated and unstable. I also don't like an openscad binary in this repo.
FWIW, I had also been working on adding a github action. You can see my WIP branch for that here: https://github.com/kennetek/gridfinity-rebuilt-openscad/compare/main...pimlie:test-use-bash-github-yaml?expand=1 It's still a WIP as I was testing the workflow with act but ran into an issue with the work folder. Not sure if it's worth continuing that branch, but to explain my choices:
But I will hold off on working on my changes for now as to now 'compete' with the work from this pr. |
@pimlie I suspect it's partly also a methodological approach.
Future Work
|
Btw, I looked at quite a bit of other SCAD projects and I didn't find any other projects yet that have fully automated OpenSCAD tests. Have you found any? Cause it would also be nice if we wouldn't have to (re-)invent the wheel for that... Anyway, don't let my remarks/suggestions keep you from implementing the tests the way you feel is best :) |
Just checking into what’s blocking this. Imo the comments are addressed and I’m always in favor of running tests in CI (even in a decreased capacity). Time is cheaper spent in CI than debugging after a break |
Merging requires at least one approving reviewer. Which has not happened. There are also some merge conflicts I need to address, and I have left it in draft status for the moment. Right now, I'm focusing on fixing some bugs in the code itself, and getting CI/CD working again. |
Gotcha, no pressure just was trying to see if there was a place to help if I had free time |
No description provided.